Skip to content

Mpt with rocksdb#1

Open
PsychoPunkSage wants to merge 17 commits intomulti-vm-experimentfrom
mpt-with-rocksdb
Open

Mpt with rocksdb#1
PsychoPunkSage wants to merge 17 commits intomulti-vm-experimentfrom
mpt-with-rocksdb

Conversation

@PsychoPunkSage
Copy link

This PR is related to experiment-2 in Multi-VM-Experiment

Signed-off-by: Abhinav Prakash <abhinav.prakash319@gmail.com>
Signed-off-by: Abhinav Prakash <abhinav.prakash319@gmail.com>
@vpanchal-supra vpanchal-supra self-requested a review April 21, 2025 06:58
@@ -0,0 +1,175 @@
use reth_db_api::{table::Table, DatabaseError};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file seems stale because we are not using any content of this file anywhere

@@ -0,0 +1,222 @@
use metrics::{Counter, Gauge, Histogram};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also seems stale

/// Write options
write_opts: WriteOptions,
/// Marker for transaction type
_marker: PhantomData<bool>,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whats the use-case?

}

/// Get the column family handle for a table
fn get_cf<T: Table>(&self) -> Result<CFPtr, DatabaseError> {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this function returns raw pointer of cf, instead we can also reference

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember why I did that... but is was related to internal impl of ColumnFamily which prevents mutable references ig (not remember exactly). Initially I was using references... but later on had to revert to this..

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this function in used in that places where the reference of cf is created from raw pointer, so I think we can simply replace raw pointer with reference.

RocksTrieCursorFactory::new(Box::leak(tx))
}

pub fn hashed_cursor_factory(&self) -> RocksHashedCursorFactory<'_>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this function is only expect to be invoked for read modded RocksTransaction so it's would be good to only implement this function for true value of WRITE

// Convert the raw pointer back to a reference safely
// This is safe as long as the DB is alive, which it is in this context
let cf_ptr = self.get_cf::<T>()?;
let cf = unsafe { &*cf_ptr };

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this unsafe code can be easily replaced with safe code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any safe way to deal with Raw-pointers? Aren't they come other unsafe section of Rust?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using raw pointer we can use simple reference

Signed-off-by: Abhinav Prakash <abhinav.prakash319@gmail.com>
Signed-off-by: Abhinav Prakash <abhinav.prakash319@gmail.com>
Signed-off-by: Abhinav Prakash <abhinav.prakash319@gmail.com>
Signed-off-by: Abhinav Prakash <abhinav.prakash319@gmail.com>
Signed-off-by: Abhinav Prakash <abhinav.prakash319@gmail.com>
Signed-off-by: Abhinav Prakash <abhinav.prakash319@gmail.com>
Signed-off-by: Abhinav Prakash <abhinav.prakash319@gmail.com>
Signed-off-by: Abhinav Prakash <abhinav.prakash319@gmail.com>
Signed-off-by: Abhinav Prakash <abhinav.prakash319@gmail.com>
Signed-off-by: Abhinav Prakash <abhinav.prakash319@gmail.com>
Signed-off-by: Abhinav Prakash <abhinav.prakash319@gmail.com>
@PsychoPunkSage
Copy link
Author

@vpanchal-supra
Made all the necessary changes... please have a look at it

Signed-off-by: Abhinav Prakash <abhinav.prakash319@gmail.com>
Signed-off-by: Abhinav Prakash <abhinav.prakash319@gmail.com>
Signed-off-by: Abhinav Prakash <abhinav.prakash319@gmail.com>
Signed-off-by: Abhinav Prakash <abhinav.prakash319@gmail.com>
@PsychoPunkSage
Copy link
Author

@vpanchal-supra I have started some review regarding the error. Please have a look at it.

Thank You

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants